Generate and validate per-operation hashes As part of securing the HTTP-based updates, we want to add a SHA256 hash of the data blob for each operation so that they can't be tampered with by a man in the middle. This CL adds support for generating and including such hashes for each operation in the payload as well as validating them in update_engine, if present. BUG=chromium-os:34298 TEST=Tested on ZGB to make sure existing functionality works fine. Existing unit tests cover all the new code paths. Change-Id: Ie42ed1930a66ceaf183f36ce3af0dea719e44237 Reviewed-on: https://gerrit.chromium.org/gerrit/33389 Reviewed-by: Don Garrett <dgarrett@chromium.org> Commit-Ready: Jay Srinivasan <jaysri@chromium.org> Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/delta_diff_generator.cc b/delta_diff_generator.cc index 06952c6..26f3133 100644 --- a/delta_diff_generator.cc +++ b/delta_diff_generator.cc
@@ -1178,6 +1178,9 @@ ssize_t rc = pread(in_fd, &buf[0], buf.size(), op->data_offset()); TEST_AND_RETURN_FALSE(rc == static_cast<ssize_t>(buf.size())); + // Add the hash of the data blobs for this operation + TEST_AND_RETURN_FALSE(AddOperationHash(op, buf)); + op->set_data_offset(out_file_size); TEST_AND_RETURN_FALSE(writer.Write(&buf[0], buf.size())); out_file_size += buf.size(); @@ -1185,6 +1188,19 @@ return true; } +bool DeltaDiffGenerator::AddOperationHash( + DeltaArchiveManifest_InstallOperation* op, + const vector<char>& buf) { + OmahaHashCalculator hasher; + + TEST_AND_RETURN_FALSE(hasher.Update(&buf[0], buf.size())); + TEST_AND_RETURN_FALSE(hasher.Finalize()); + + const vector<char>& hash = hasher.raw_hash(); + op->set_data_sha256_hash(hash.data(), hash.size()); + return true; +} + bool DeltaDiffGenerator::ConvertCutToFullOp(Graph* graph, const CutEdgeVertexes& cut, const string& new_root, diff --git a/delta_diff_generator.h b/delta_diff_generator.h index b646545..f4717e7 100644 --- a/delta_diff_generator.h +++ b/delta_diff_generator.h
@@ -170,6 +170,15 @@ const std::string& data_blobs_path, const std::string& new_data_blobs_path); + // Computes a SHA256 hash of the given buf and sets the hash value in the + // operation so that update_engine could verify. This hash should be set + // for all operations that have a non-zero data blob. One exception is the + // dummy operation for signature blob because the contents of the signature + // blob will not be available at payload creation time. So, update_engine will + // gracefully ignore the dummy signature operation. + static bool AddOperationHash(DeltaArchiveManifest_InstallOperation* op, + const std::vector<char>& buf); + // Handles allocation of temp blocks to a cut edge by converting the // dest node to a full op. This removes the need for temp blocks, but // comes at the cost of a worse compression ratio.
diff --git a/delta_performer.cc b/delta_performer.cc index 46739b1..d0a38e0 100644 --- a/delta_performer.cc +++ b/delta_performer.cc
@@ -340,19 +340,37 @@ return true; } - *error = ValidateOperationHash(op); - if (*error != kActionCodeSuccess) { - // Cannot proceed further as operation hash is invalid. - // Higher level code will take care of retrying appropriately. - return false; + bool should_log = (next_operation_num_ % 1000 == 0 || + next_operation_num_ == total_operations - 1); + + // Validate the operation only if the manifest signature is present. + // Otherwise, keep the old behavior. This serves as a knob to disable + // the validation logic in case we find some regression after rollout. + if (!install_plan_->manifest_signature.empty()) { + // Note: Validate must be called only if CanPerformInstallOperation is + // called. Otherwise, we might be failing operations before even if there + // isn't sufficient data to compute the proper hash. + *error = ValidateOperationHash(op, should_log); + if (*error != kActionCodeSuccess) { + // Cannot proceed further as operation hash is invalid. + // Higher level code will take care of retrying appropriately. + // + // TODO(jaysri): Add a UMA stat to indicate that an operation hash + // was failed to be validated as expected. + // + // TODO(jaysri): VALIDATION: For now, we don't treat this as fatal. + // But once we're confident that the new code works fine in the field, + // we should uncomment the line below. + // return false; + LOG(INFO) << "Ignoring operation validation errors for now"; + } } // Makes sure we unblock exit when this operation completes. ScopedTerminatorExitUnblocker exit_unblocker = ScopedTerminatorExitUnblocker(); // Avoids a compiler unused var bug. // Log every thousandth operation, and also the first and last ones - if ((next_operation_num_ % 1000 == 0) || - (next_operation_num_ + 1 == total_operations)) { + if (should_log) { LOG(INFO) << "Performing operation " << (next_operation_num_ + 1) << "/" << total_operations; } @@ -719,10 +737,70 @@ } ActionExitCode DeltaPerformer::ValidateOperationHash( - const DeltaArchiveManifest_InstallOperation& - operation) { + const DeltaArchiveManifest_InstallOperation& operation, + bool should_log) { - // TODO(jaysri): To be implemented. + if (!operation.data_sha256_hash().size()) { + if (!operation.data_length()) { + // Operations that do not have any data blob won't have any operation hash + // either. So, these operations are always considered validated since the + // manifest that contains all the non-data-blob portions of the operation + // has already been validated. + return kActionCodeSuccess; + } + + // TODO(jaysri): Add a UMA stat here so we're aware of any + // man-in-the-middle attempts to bypass these checks. + // + // TODO(jaysri): VALIDATION: no hash is present for the operation. This + // shouldn't happen normally for any client that has this code, because the + // corresponding update should have been produced with the operation + // hashes. But if it happens it's likely that we've turned this feature off + // in Omaha rule for some reason. Once we make these hashes mandatory, we + // should return an error here. + // One caveat though: The last operation is a dummy signature operation + // that doesn't have a hash at the time the manifest is created. So we + // should not complaint about that operation. This operation can be + // recognized by the fact that it's offset is mentioned in the manifest. + if (manifest_.signatures_offset() && + manifest_.signatures_offset() == operation.data_offset()) { + LOG(INFO) << "Skipping hash verification for signature operation " + << next_operation_num_ + 1; + } else { + // TODO(jaysri): Uncomment this logging after fixing dev server + // LOG(WARNING) << "Cannot validate operation " << next_operation_num_ + 1 + // << " as no expected hash present"; + } + return kActionCodeSuccess; + } + + vector<char> expected_op_hash; + expected_op_hash.assign(operation.data_sha256_hash().data(), + (operation.data_sha256_hash().data() + + operation.data_sha256_hash().size())); + + OmahaHashCalculator operation_hasher; + operation_hasher.Update(&buffer_[0], operation.data_length()); + if (!operation_hasher.Finalize()) { + LOG(ERROR) << "Unable to compute actual hash of operation " + << next_operation_num_; + return kActionCodeDownloadOperationHashVerificationError; + } + + vector<char> calculated_op_hash = operation_hasher.raw_hash(); + if (calculated_op_hash != expected_op_hash) { + LOG(ERROR) << "Hash verification failed for operation " + << next_operation_num_ << ". Expected hash = "; + utils::HexDumpVector(expected_op_hash); + LOG(ERROR) << "Calculated hash over " << operation.data_length() + << " bytes at offset: " << operation.data_offset() << " = "; + utils::HexDumpVector(calculated_op_hash); + return kActionCodeDownloadOperationHashMismatch; + } + + if (should_log) + LOG(INFO) << "Validated operation " << next_operation_num_ + 1; + return kActionCodeSuccess; } diff --git a/delta_performer.h b/delta_performer.h index 8ea30c3..a9d70e7 100644 --- a/delta_performer.h +++ b/delta_performer.h
@@ -155,7 +155,7 @@ // matches what's specified in the manifest in the payload. // Returns kActionCodeSuccess on match or a suitable error code otherwise. ActionExitCode ValidateOperationHash( - const DeltaArchiveManifest_InstallOperation& operation); + const DeltaArchiveManifest_InstallOperation& operation, bool should_log); // Interprets the given |protobuf| as a DeltaArchiveManifest protocol buffer // of the given protobuf_length and verifies that the signed hash of the
